New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for APP_ENV environment variable #2702
Add support for APP_ENV environment variable #2702
Conversation
test/test_cli.rb
Outdated
|
||
Puma::CLI.new [] | ||
|
||
assert_equal 'test', ENV['RACK_ENV'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_equal 'test', cli.environment
test/test_cli.rb
Outdated
ENV['RAILS_ENV'] = @environment | ||
ENV['APP_ENV'] = 'test' | ||
|
||
Puma::CLI.new [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli = Puma::CLI.new []
One change, otherwise LGTM and thanks for implementing! |
Using APP_ENV to set the environment is supported by Sinatra and Sidekiq, so it makes sense to support the same behavior in Puma. Like Sidekiq, APP_ENV will take precedence over RACK_ENV and RAILS_ENV. APP_ENV defers to any argument passed via the --environment flag. Closes puma#2692
ee2a68e
to
ff6f08c
Compare
@nateberkopec Updated and force pushed the branch! |
@nateberkopec It looks like |
Fixed it up: f21ae53 I couldn't remember the API. What I was trying to get across was that the original test didn't actually test anything, it was making assertions about the setup and not about the action you took during the test. Unfortunately all the other tests around this behavior are exactly the same (I just deleted them). |
@nateberkopec I see. I was curious about how this worked, but I (silly of me) assumed the old tests were trustworthy. I'll look into writing some new tests to cover the old cases, thanks for your help! |
That would be great! If you want to refactor how the CLI object works a bit too so we don't have to Puma's an old project... some of the tests are good, others are not 😆 I don't blame you for following what was already there. |
Description
Using APP_ENV to set the environment is supported by Sinatra and
Sidekiq, so it makes sense to support the same behavior in Puma.
Like Sidekiq, APP_ENV will take precedence over RACK_ENV and RAILS_ENV.
APP_ENV defers to any argument passed via the --environment flag.
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.